Skip to content

Conversation

@giacomofiorin
Copy link
Member

In draft for now: even with the default settings, the current code does not pass the test.

@giacomofiorin giacomofiorin added the testing Only affects CI; not listed in outside PRs label Sep 9, 2025
@giacomofiorin giacomofiorin force-pushed the clang-tidy branch 2 times, most recently from 47a2951 to af5e7f9 Compare September 9, 2025 20:46
@giacomofiorin
Copy link
Member Author

@HanatoK Continuing the conversation here, one of the advantages of using a container is to use a recent version of the Clang tools. The ubuntu-latest GitHub instance runs 18, but CentOS 9 comes with version 20.

@giacomofiorin
Copy link
Member Author

With the downgrade from Clang 20 to 18 and removal of the Lepton sources, we are down to two issues, one with colvarproxy_io::io_available() and one with the bitwise OR of values of Parse_Mode type.

I have a good idea about how to fix the former (requires some small refactoring) but I'm puzzled by the latter: I though that having added the clang::flag_enum annotation would have taken care of it, but was wrong. Any ideas?

@giacomofiorin
Copy link
Member Author

Looks like the checks are the same between 18 and 20, but good to have the latter back for the future (thanks @HanatoK).

I would like to add support for GNU Parallel in run_clang_tidy.sh. (The original command run-clang-tidy had the advantage of a -j flag.) What do you think?

@HanatoK
Copy link
Member

HanatoK commented Sep 10, 2025

Looks like the checks are the same between 18 and 20, but good to have the latter back for the future (thanks @HanatoK).

I would like to add support for GNU Parallel in run_clang_tidy.sh. (The original command run-clang-tidy had the advantage of a -j flag.) What do you think?

That's a good idea, but I don't know how to parallelize the script with GNU Parallel. It looks like there are other ways to parallelize the executions as well (see https://stackoverflow.com/questions/38774355/how-to-parallelize-for-loop-in-bash-limiting-number-of-processes).

@giacomofiorin
Copy link
Member Author

That's a good idea, but I don't know how to parallelize the script with GNU Parallel. It looks like there are other ways to parallelize the executions as well (see https://stackoverflow.com/questions/38774355/how-to-parallelize-for-loop-in-bash-limiting-number-of-processes).

I had forgotten about xargs -P: it's less powerful than Parallel, but more widely available. I added it to the script.

Also restored the -header-filter flag, so that we can see warnings from headers (there's a bunch in colvar_UIestimator.h

@jhenin
Copy link
Member

jhenin commented Oct 13, 2025

/home/runner/work/colvars/colvars/src/colvarproxy_io.cpp:493:9: note: Call to virtual method 'colvarproxy_io::io_available' during destruction bypasses virtual dispatch
493 | if (! io_available()) {

This seems hard to fix. There is no safe way to call the derived class virtual methods from the base class destructor. I suppose io_available could be a flag in the base class that is set by the derived class, and can be checked by the base class destructor. But the close_output_streams is also virtual, so I imagine we'll have the same issue.

To solve this we could either have a sort of "anticipated destruction" like deferred initialization but the other way, or transfer that logic to the derived class destructors, at the code of code duplication, which sucks. Maybe anticipated destruction (finalization) could be done using RAII?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Only affects CI; not listed in outside PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants